direct: Fix "Nothing to update" when a UC comment is cleared or set outside the bundle - #6343
Open
denik wants to merge 16 commits into
Open
direct: Fix "Nothing to update" when a UC comment is cleared or set outside the bundle#6343denik wants to merge 16 commits into
denik wants to merge 16 commits into
Conversation
Collaborator
Integration test reportCommit: 35cab36
Top 3 slowest tests (at least 2 minutes):
|
denik
force-pushed
the
denik/issue-6340
branch
from
August 24, 2026 12:19
e9bd2fc to
02ee8cd
Compare
denik
added a commit
that referenced
this pull request
Aug 24, 2026
## Why We already pass it to DoUpdate, so it was just an omission. Planned to be used in #6343
denik
force-pushed
the
denik/issue-6340
branch
3 times, most recently
from
August 25, 2026 07:29
0596606 to
1457ca6
Compare
…he bundle A schema without `comment` in the config becomes undeployable once someone sets a description on it in UC: the engine reads the remote comment, plans an update, and every field of the PATCH serializes away under omitempty. UC rejects the empty body with `400 INVALID_PARAMETER_VALUE / UpdateSchema Nothing to update`, which aborts the whole deploy. Add an acceptance test for it and make the fake workspace reject an empty UpdateSchema payload the way UC does, so the local run fails the same way the cloud one does. Co-authored-by: Isaac
…n empty PATCH Every UpdateSchema field is omitempty, so a schema whose config declares no comment produced an empty PATCH body once the comment was set out of band. UC answers that with `400 / UpdateSchema Nothing to update` rather than a no-op, which failed the whole deploy with no way out from the CLI. Force-send comment so the payload always carries a field and clearing a comment set outside the bundle actually happens. Co-authored-by: Isaac
Co-authored-by: Isaac
Catalogs and volumes fail exactly like schemas did: their update payloads carry only fields the config may leave unset, so clearing a comment that was set out of band produced an empty PATCH and `400 / Nothing to update`. Verified against a real workspace for both. Force-send comment in all four update paths (catalogs and volumes each have a rename variant), moving the shared reason into forceSendComment, and teach the fake workspace to reject an empty payload and honour an explicit empty comment the way UC does. Co-authored-by: Isaac
The bundle name is the workspace state path, and cloud tests share one real workspace, so the hardcoded "test-bundle" made these three fight over the same deploy.lock as every other test using that name. They passed run alone and failed under parallelism: the integration run reported success while retrying them on nearly every environment. Co-authored-by: Isaac
…itionally The previous commits always force-sent comment, so a deploy whose only drift was another field still put `"comment": ""` on the wire, and a field the plan had classified as skip would have been cleared behind the plan's back. Any other omitempty field the config stops setting was still dropped, so it never converged. Derive the force-send list from the plan instead, matched against the request type's own JSON names. Each PATCH now carries exactly what the plan says is changing, and roughly 40 omitempty fields across the UC resources converge instead of only comment. forceSendClearedFields documents the general shape, including why full-replacement APIs (jobs, pipelines, model serving) need none of it. Also drop the special-casing in the fake workspace: applyUpdatedFields applies whatever the payload names, including zero values, which is what a partial-update API does. That incidentally stops mergo from clobbering the stored ForceSendFields, so browse_only survives an update the way UC returns it. Co-authored-by: Isaac
… locations Removing a comment from the configuration reports a successful update and then never converges: the field is omitempty, so it leaves the payload entirely, and a partial-update API reads its absence as "leave unchanged". The comment stays, and every later plan reports the same pending change. Unlike the schema case this needs no out-of-band edit, and there is no error to notice -- the deploy says "1 changed" while changing nothing. The external locations golden also gains four synced files, since the new test lives inside that test's bundle root. Co-authored-by: Isaac
… too Apply forceSendClearedFields to the two remaining resources whose update drops a cleared field, so removing a comment from the configuration converges instead of reporting a change forever. External locations get it on both the update and the rename path. The fake workspace applies whatever an update payload names, replacing guards that swallowed an explicit empty comment. Registered models accept a payload with no field at all -- unlike schemas, catalogs and volumes, verified against a real workspace -- so parseUCUpdate is split and they use the parse-only half rather than gaining a rejection the backend does not have. Co-authored-by: Isaac
main removed the setting in #6359 while this branch was open, so the merge failed with "Undecoded key ... RequiresUnityCatalog" on the direct-engine runners. Co-authored-by: Isaac
resources/<resource>/<test-name> matches the equivalent tests that already live flat (cluster_policies/out_of_band_change, apps/config-drift, permissions/jobs/added_remotely), and the drift prefix says nothing the names do not. Pure rename, no content change. Co-authored-by: Isaac
grep -v silently leaves the config unchanged if the pattern stops matching, and these tests would then assert convergence against a config that still declares the comment. update_file.py fails instead, and the intermediate copies of databricks.yml are no longer needed. Co-authored-by: Isaac
External locations already did this; catalogs and volumes did not, so a deploy that renamed one of them dropped a field the plan reported as cleared. The stale comment left behind claimed DoUpdateWithID cannot see the plan, which stopped being true when #6360 landed. Co-authored-by: Isaac
An adversarial review found that removing a config-set owner from a registered model made the plan report an actionable change, so the cleared value was force-sent as owner: "" -- which UC rejects with "Could not find principal with name .". backend_defaults does not cover it: that rule only skips when old and new are both nil. Route the cleared fields through the same FilterFields exclusions as config.ForceSendFields, so the exclusion list each request already declares is authoritative for both. The comment claiming owner is not in the configuration tree was wrong: it is, via the embedded CreateRegisteredModelRequest. Also record the whole changes map in the drift goldens rather than just comment, and stop the fake workspace accumulating duplicate ForceSendFields entries. Co-authored-by: Isaac
Deriving from entry.Changes made every omitempty field a candidate, so the default for an unverified field was to send its zero value -- the hazard #6088 was about ('' is not a valid cluster policy ID). It also went against dresources/README.md, which asks for a static list rather than field names derived from the plan. Each resource now names the fields it force-sends, and only comment qualifies. Three things have to hold, and the last one is easy to miss: the backend must accept the zero value as a clear (rules out owner and new_name), ForceSendFields must affect the field at all (rules out maps such as properties), and terraform must send it too (rules out custom_max_retention_hours, which UC does clear on 0 but terraform never sends, so force-sending it made the engines disagree). Co-authored-by: Isaac
denik
force-pushed
the
denik/issue-6340
branch
from
August 25, 2026 14:01
408bc92 to
8fb90d2
Compare
Removing the retention section left the file ending in a blank line, which the whitespace linter fixes and CI then fails on. Co-authored-by: Isaac
Recording the whole changes map surfaced the backend-generated storage_location, whose scheme and bucket differ per cloud, so the azure and gcp integration runs failed on an s3:// golden. Normalize to the AWS form ahead of the METASTORE_NAME repl, the same way grants/volumes does. Verified against azure and gcp. ForceSendFields is now assigned inline: the two-step only existed because the old reflection helper had to inspect the built request. print_requests.py moves out of the cleanup trap to just after the deploys, with the trap consuming the destroy's requests so none are left recorded. Co-authored-by: Isaac
denik
marked this pull request as ready for review
August 25, 2026 18:54
Contributor
Approval status: pending
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
A schema that does not declare
commentbecomes undeployable as soon as someonesets a description on it in UC. The plan clears the comment, every
UpdateSchemafield is omitempty, and the empty PATCH is rejected:
Catalogs and volumes fail the same way. Registered models and external locations
always send other fields, so there is nothing to reject — instead removing a
comment from the configuration reports "1 changed" and never converges.
Changes
Each of the five resources names the fields its update force-sends, so a value the
config stops declaring is sent as the zero value instead of being dropped. Only
commentqualifies: the backend has to accept the zero value as a clear (owner: ""and
new_name: ""are both rejected),ForceSendFieldshas to affect the field atall (it is inert for maps, so
propertiescannot be cleared by any payload), andterraform has to send it too (
custom_max_retention_hoursfails that one).backend_defaultswould be wrong here: a description typed in Catalog Explorer isreal drift, not a value the backend filled in, and suppressing it would stop the
bundle from managing
comment.Fixes #6340
This pull request and its description were written by Isaac.